Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cpp: Store compounds real bit size with its pos #658

Conversation

johannes-wolf
Copy link
Contributor

@johannes-wolf johannes-wolf commented Aug 20, 2024

Please check contribution guide first.

Description

With the implementation of bitPosition(), I did not take relative encoded arrays/packed objects into account. The proposed solution is to store the "physical bit-size" of an object along with its bit-position, because we cannot call bitSizeOf(context, offset) at a later stage.

This PR adds a second field m_realBitSize that stores the physical bit-size of an object on construction.

I am not sure about the name realBitPosition(), maybe physicalSizeOf() or packedSizeOf() is better?

Tests are yet missing. I will add them to this PR.

Type of Change

  • Bug fix
  • New feature
  • Documentation enhancement

@johannes-wolf johannes-wolf added the c++ C++ language generator label Aug 20, 2024
@johannes-wolf johannes-wolf force-pushed the bit-position-remember-real-bit-size branch from b31e47f to 9f30008 Compare August 20, 2024 12:44
@mikir
Copy link
Contributor

mikir commented Sep 4, 2024

Thank you for pointing this out! Packed arrays are indeed a problem.

The naming is always the problem, so I would suggest to implement only one method to return structure with all information:

::zserio::ParsingInfo parsingInfo() const;
struct ParsingInfo
{
    size_t bitPosition;
    size_t bitSize;
};

This will allow to add more parsing information in the future without any change in the design. If this suggestion is ok for you, we will implement it.

But before we do this, I would like to check with you the following:

  1. Using the current solution, there is no way how to get the bit position and the bit size of the single field in the compound. Is this ok for your use case?
  2. Let's suppose that we will have enough capacity to implement more robust solution. This solution will implement for example read constructor overload which will return hash table which will map Zserio object to Parsing Info. Would it be such solution acceptable for your use case?

@johannes-wolf
Copy link
Contributor Author

johannes-wolf commented Sep 4, 2024

Thank you for pointing this out! Packed arrays are indeed a problem.

The naming is always the problem, so I would suggest to implement only one method to return structure with all information:

::zserio::ParsingInfo parsingInfo() const;

Sounds good! Shall I change that with this PR?

struct ParsingInfo
{
    size_t bitPosition;
    size_t bitSize;
};

This will allow to add more parsing information in the future without any change in the design. If this suggestion is ok for you, we will implement it.

But before we do this, I would like to check with you the following:

1. Using the current solution, there is no way how to get the bit position and the bit size of the single field in the compound. Is this ok for your use case?

Yes, getting the bit-position for non-compound fields is not needed.

2. Let's suppose that we will have enough capacity to implement more robust solution. This solution will implement for example read constructor overload which will return hash table which will map Zserio object to Parsing Info. Would it be such solution acceptable for your use case?

What would be the key used for hashing, and what would be the reason to store the ParsingInfo external to the objects? Memory consumption? I guess such an implementation would work for us.

@mikir
Copy link
Contributor

mikir commented Sep 4, 2024

::zserio::ParsingInfo parsingInfo() const;

Sounds good! Shall I change that with this PR?

Thanks a lot but it is ok, we will do it.

  1. Let's suppose that we will have enough capacity to implement more robust solution. This solution will implement for example read constructor overload which will return hash table which will map Zserio object to Parsing Info. Would it be such solution acceptable for your use case?

What would be the key used for hashing, and what would be the reason to store the ParsingInfo external to the objects? Memory consumption? I guess such an implementation would work for us.

The key will be probably the pointer to the Zserio object. The main reason would be to avoid of the possible parsingInfo misuse using the current solution. If we return such hash table only after parsing, there will no way how to call it wrongly (for example, if you create Zserio object manually without calling of read constructor and call bitPosition, you will get wrong results). My idea is that if turns out in the future that this feature is important and should be part of the stable API, we might change the design in this way if it is acceptable for you (no big changes on your side).

@mikir mikir changed the base branch from master to issue-652-ci September 6, 2024 06:15
@mikir mikir merged commit 3e51e73 into ndsev:issue-652-ci Sep 6, 2024
56 of 63 checks passed
mikir added a commit that referenced this pull request Sep 18, 2024
mikir pushed a commit that referenced this pull request Sep 20, 2024
* cpp: Store compounds real bit size with its pos

* Update clang-tidy suppressions
mikir added a commit that referenced this pull request Sep 20, 2024
mikir pushed a commit that referenced this pull request Sep 24, 2024
* cpp: Store compounds real bit size with its pos

* Update clang-tidy suppressions
mikir added a commit that referenced this pull request Sep 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ C++ language generator
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants